Skip to content

Remove usage of npx #4003

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 8, 2025
Merged

Remove usage of npx #4003

merged 1 commit into from
Jun 8, 2025

Conversation

@user202729
Copy link
Contributor

@user202729 user202729 commented May 26, 2025 •
edited by ellipsis-dev bot
Loading

Related GitHub Issue

None yet

Description

Currently the build script depends on npx. This is unnecessary (look at other scripts, they don't use npx and work just fine), and also currently Roo-Code does not have npx as a dependency.

Test Procedure

pnpm build creates the .vsix file for me without having npx installed.

Type of Change

  • 🐛 Bug Fix: Non-breaking change that fixes an issue.
  • ✨ New Feature: Non-breaking change that adds functionality.
  • 💥 Breaking Change: Fix or feature that would cause existing functionality to not work as expected.
  • ♻️ Refactor: Code change that neither fixes a bug nor adds a feature.
  • 💅 Style: Changes that do not affect the meaning of the code (white-space, formatting, etc.).
  • 📚 Documentation: Updates to documentation files.
  • ⚙️ Build/CI: Changes to the build process or CI configuration.
  • 🧹 Chore: Other changes that don't modify src or test files.

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Code Quality:
    • My code adheres to the project's style guidelines.
    • There are no new linting errors or warnings (npm run lint).
    • All debug code (e.g., console.log) has been removed.
  • Testing:
    • New and/or updated tests have been added to cover my changes.
    • All tests pass locally (npm test).
    • The application builds successfully with my changes.
  • Branch Hygiene: My branch is up-to-date (rebased) with the main branch.
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Changeset: A changeset has been created using npm run changeset if this PR includes user-facing changes or dependency updates.
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos

Documentation Updates

Additional Notes

Get in Touch

Important

Remove npx from vsix script in package.json, simplifying the build process.

  • Build Script:
    • Remove npx from vsix script in package.json, directly using vsce package instead.

This description was created by Ellipsis for 3f0a689c974083b646b9996ffa5211f1dc410b24. You can customize this summary. It will automatically update as commits are pushed.

@user202729 user202729 requested review from cte and mrubens as code owners May 26, 2025 17:36
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label May 27, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Preliminary Review] in Roo Code Roadmap May 28, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels May 28, 2025
@daniel-lxs
Copy link
Member

daniel-lxs commented May 30, 2025 •
edited
Loading

Hey @user202729, Thank you for your contribution! While the change itself is valid, using npx provides a fallback mechanism when vsce (which is currently a dev dependency) isn't installed globally. Removing it could break builds for some developers.

I think we should create an issue and discuss if this change makes sense before committing to it, I'll be closing this issue temporarily but feel free to reopen it once we have a discussion going.

@daniel-lxs daniel-lxs closed this May 30, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Prelim Review] to Done in Roo Code Roadmap May 30, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap May 30, 2025
@user202729
Copy link
Contributor Author

user202729 commented May 31, 2025

No, npm script automatically adds node_modules/.bin to PATH. In fact, I don't have vsce installed globally and it works just fine.

See

Also, as I mentioned in the pull request description: other scripts are already using this. two lines above:

		"publish:marketplace": "vsce publish --no-dependencies && ovsx publish --no-dependencies",

@daniel-lxs daniel-lxs moved this from Done to PR [Needs Prelim Review] in Roo Code Roadmap May 31, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to Done in Roo Code Roadmap May 31, 2025
@daniel-lxs
Copy link
Member

daniel-lxs commented May 31, 2025

@user202729 Thank your for your response.

Is there any downside to having npx there at all? other than it being unnecessary?

@user202729
Copy link
Contributor Author

user202729 commented May 31, 2025

the downside is if someone doesn't have npx installed (such as me) and follow the instructions in the README exactly to build the package, they will get the error message that npx is not found. It's arguably a bug in the code if following the instructions exactly lead to an error raised.

@daniel-lxs
Copy link
Member

daniel-lxs commented May 31, 2025

@user202729
I agree, let me reopen this so we can review it, thank you!

@daniel-lxs daniel-lxs reopened this May 31, 2025
@github-project-automation github-project-automation bot moved this from Done to Triage in Roo Code Roadmap May 31, 2025
@github-project-automation github-project-automation bot moved this from Done to New in Roo Code Roadmap May 31, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap May 31, 2025
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 1, 2025
daniel-lxs
daniel-lxs approved these changes Jun 5, 2025
View reviewed changes
Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked if I was able to run the script without having vsce installed globally and it seems to work normally.

cc: @cte

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 5, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Jun 5, 2025
@mrubens mrubens merged commit 73e4418 into RooCodeInc:main Jun 8, 2025
15 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jun 8, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Jun 8, 2025
@hannesrudolph
Copy link
Collaborator

hannesrudolph commented Jun 18, 2025

@user202729 do you have discord?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrubens mrubens mrubens approved these changes

@daniel-lxs daniel-lxs daniel-lxs approved these changes

@cte cte Awaiting requested review from cte cte is a code owner

Assignees

No one assigned

Labels

lgtm This PR has been approved by a maintainer PR - Needs Review size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

Roo Code Roadmap
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@user202729 @daniel-lxs @hannesrudolph @mrubens